fix: check simulation call trace for reversion #1861
Conversation
Branch preview✅ Deploy successful! https://simulation_call_trace--webcore.review-web-core.5afe.dev |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| return [] | ||
| } | ||
|
|
||
| return simulation.transaction.call_trace.filter((call) => call.error) |
There was a problem hiding this comment.
Why do we go for call errors instead of the ExecutionFailure event as written in the comment?
We could iterate over simulation.transaction.logs and filter by log.name === 'ExecutionFailure' && log.raw.address === safeAddress emitted from safeAddress.
There was a problem hiding this comment.
For future reference: after discussing this, differing Safe versions emit different events (<1.1.0 emits ExecutionFailed) so the logs aren't consistent.
As such, we've adjusted the code to only extract the call traces if the transaction simulation "succeeds".
| The transaction failed during the simulation{' '} | ||
| {isCallTraceError ? ( | ||
| <> | ||
| with error <b>{callTraceErrors[0].error}</b>. |
There was a problem hiding this comment.
I think this leads to too technical errors. The one in the PRs screenshot for instance reads "value: execution error" which does not really help users IMO.
I would just scan for the event log and give a generic error here OR if we want to give more details on what failed, we could start collecting common call_trace errors and map them to human readable error messages.
For the out of gas error for instance this error is present in the call traces:
"error": "out of gas" which we could rephrase to "The transaction failed with an out of gas error. Try increasing the safeTxGas and try again."
There was a problem hiding this comment.
For future reference: we've generalised the error messages in accordance with the above clarification.
|
In general I think this is a great addition to our simulations as it is quite confusing that they show up as "Successful" although they revert in the assembly block of our contracts. We could also reach out to tenderly about this issue. |
|
Hola team! The improvement is working nicely. Only issue I see is that it will conflict with the current tenderly simulation that it is performed on the app itself as the behaviour there will still be the old one. I don't know if this change should be integrated on the app side as well to avoid cases like this: sim.mp4Cheers! |
The Transaction Builder needs to be updated to include the changes in this PR. I'll create an issue for it to be tackled now. Edit: I've created an issue. |
|
Gracias señor! All good from me to be merged 😎 |
What it solves
Resolves successful simulations that should fail.
How this PR fixes it
The simulation
call_traceis also checked for errors if the Tenderly simulation is successful. Example simulation can be found here.How to test it
Open a 1.1.1 Safe on Gnosis Chain and create the following batch in the Transaction Builder:
toof current Safe,changeMasterCopyto0x3E5c63644E683549055b9Be8653de26E0B4CD36Etoof Current Safe,setMasterCopyto0xf48f2B2d2a534e402487b3ee7C18c33Aec0Fe5e4toof0x7a48Dac683DA91e4faa5aB13D91AB5fd170875bd,swapOwnertoof0x7a48Dac683DA91e4faa5aB13D91AB5fd170875bd,swapOwnerfrom to-be-added owner added in 3.Simulate transaction (in the creation modal) and observe the simulation failing with:
Screenshots
Checklist